Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rework load progress dialog #918

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Rework load progress dialog #918

wants to merge 1 commit into from

Conversation

pjonsson
Copy link
Contributor

@pjonsson pjonsson commented Dec 2, 2022

This uses the Java builtin ProgressMonitor class. As an additional bonus, this makes the cancel button work.

@pjonsson pjonsson force-pushed the gui-nonstatic-warnings branch from 8da04dc to 6ae6d96 Compare December 2, 2022 22:24
@pjonsson pjonsson force-pushed the gui-nonstatic-warnings branch from 6ae6d96 to 3691dfd Compare January 13, 2023 19:19
@pjonsson
Copy link
Contributor Author

@nfi after the latest changes, this should work better since the AWT thread isn't as contended as before.

@pjonsson pjonsson mentioned this pull request Jan 13, 2023
Closed
@nfi
Copy link
Member

nfi commented Jan 14, 2023

Looks better. There are no longer any problems interacting with the progress dialog under macOS. Some things I noticed:

  1. If a simulation fails to load and I close the error message dialog - the progress dialog remains open and idle. And another progress dialog opens if I load the simulation again.
  2. If I cancel loading a simulation and reload the simulation, I often end up with multiple instances of the plugins. It seems the old simulation sometimes is not completely removed before the new simulation is set up. I am not sure if this issue is noticeable now due to the cancel button working better or something else, but I did not manage to reproduce this in the master. And it does not seem to happen if I wait a little before reloading the simulation, so I suspect a race condition somewhere.

@pjonsson pjonsson force-pushed the gui-nonstatic-warnings branch from 3691dfd to fad0b50 Compare January 14, 2023 01:23
@pjonsson
Copy link
Contributor Author

The first issue is fixed, was missing a setProgress when restarting/cancelling.

The second issue is hard to reproduce, even when I limit my CPU to 1700 MHz. I fixed the issue I found.

@pjonsson pjonsson force-pushed the gui-nonstatic-warnings branch 3 times, most recently from 0612e11 to 920154c Compare January 16, 2023 14:43
@pjonsson pjonsson mentioned this pull request Jan 23, 2023
Merged
@pjonsson pjonsson force-pushed the gui-nonstatic-warnings branch from 920154c to c79f71e Compare January 23, 2023 20:44
@pjonsson
Copy link
Contributor Author

The last revision worked, but I think the commit history had some rebase-leftovers. Fixed that, and everything should work in this revision.

@pjonsson pjonsson force-pushed the gui-nonstatic-warnings branch from c79f71e to a014273 Compare November 5, 2023 12:07
nfi
nfi reviewed Jan 12, 2024
View reviewed changes
java/org/contikios/cooja/GUI.java
progressDialog.setLocationRelativeTo(frame);
progressDialog.setDefaultCloseOperation(JDialog.DO_NOTHING_ON_CLOSE);
final String progressTitle = "Loading " + (configFile == null ? cooja.getSimulation().getCfg().file() : configFile.getAbsolutePath());
progressMonitor = new ProgressMonitor(frame, progressTitle, "", 0, 6);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A note with empty string takes no space and ProgressMonitor does not revalidate if a note is added later. Add an initial note to reserve space for notes in the dialog. Otherwise the 'Cancel' button is truncated in some UI such as Flatlaf.

Suggested change
progressMonitor = new ProgressMonitor(frame, progressTitle, "", 0, 6);
progressMonitor = new ProgressMonitor(frame, progressTitle, "Preparing to load simulation", 0, 6);

java/org/contikios/cooja/GUI.java
progressDialog.getRootPane().setDefaultButton(button);
progressDialog.setLocationRelativeTo(frame);
progressDialog.setDefaultCloseOperation(JDialog.DO_NOTHING_ON_CLOSE);
final String progressTitle = "Loading " + (configFile == null ? cooja.getSimulation().getCfg().file() : configFile.getAbsolutePath());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps set a better title on the progress monitor? Otherwise the title in, for example, a Swedish system will be 'Pågår'.

Suggested change
final String progressTitle = "Loading " + (configFile == null ? cooja.getSimulation().getCfg().file() : configFile.getAbsolutePath());
final String progressTitle = "Loading " + (configFile == null ? cooja.getSimulation().getCfg().file() : configFile.getAbsolutePath());
final String defaultTitle = UIManager.getString("ProgressMonitor.progressText");
UIManager.put("ProgressMonitor.progressText", "Loading simulation");

java/org/contikios/cooja/Simulation.java
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The max progress includes any deprecated plugins. Not that it will make a noticeable difference, but should there not be a Cooja.tickProgress(); before line 360 to count skipped plugins?

java/org/contikios/cooja/GUI.java
progressMonitor = new ProgressMonitor(frame, progressTitle, "", 0, 6);
progressMonitor.setMillisToDecideToPopup(0);
progressMonitor.setMillisToPopup(0);
progressMonitor.setProgress(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restore original title for ProgressMonitor if it was changed earlier.

Suggested change
progressMonitor.setProgress(0);
progressMonitor.setProgress(0);
UIManager.put("ProgressMonitor.progressText", defaultTitle);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nfi nfi nfi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pjonsson @nfi